Repository navigation
chore(web): add useLatestLoad and lint raw async effects - #1190
Conversation
|
Review round 1 (Codex
Verified: the lint selector catches nested async helpers, |
All four are fixed. Head:
The fix for 1 also closes the ceiling I had noted: the PR that enables a rule can no longer baseline a fresh violation of it. The tables are in the PR body under "Review round 1 fixes" and "The growth check on a later PR". |
|
Review round 2 (Codex All four round 1 repros are closed: the NamedEntityPicker move, save-then-retry, a synchronous throw, and a bad or bare
The review loop stops here. |
Adds one supported way for a screen to load data, and a lint that stops new raw async effects.
Closes #1185
The hook
useLatestLoad(inputs, load)inweb/src/components/, next tousePagedList. It returns{ data, error, loading, retry }.AbortController, which is also its generation. Changinginputs, callingretry, or unmounting aborts it. An aborted run sets no state, in the failure path as much as the success path.loadis read throughuseEffectEvent, so an inline arrow does not reload on every render. Onlyinputsandretryreload.datastays while a newer run loads, soloadingtells a screen it may belong to the inputs being left. A failure clears it, asusePagedListdoes: empty is honest, stale is not.The lint
no-restricted-syntaxinweb/lint/eslint.config.js, forsrc/routes/**andsrc/components/**(test files excluded). It flags anawaitor a.then(call anywhere inside auseEffect(...)/React.useEffect(...)call:There are 30 existing sites in 18 files. I read each flagged line, and none is a false positive. They are recorded in
web/lint/eslint-suppressions.jsonand none is migrated here. The hook carries the one reasonedeslint-disable, because it is the guard the rule points to.Ceiling: the rule cannot see an async helper defined outside the effect and called from it, nor a write followed by a refresh in an event handler. Only the hooks cover those.
web/AGENTS.mdsays so.The suppressions growth check needed one change
The growth check from #1155 did not allow this baseline. It refused any file/rule entry that was new against the base, so this PR's 30 new entries failed it.
check-suppressions.mjsnow has one exception. When a rule did not apply to a file under the base's own ESLint config (calculateConfigForFileon the baseeslint.config.js, presets included), that file's entry may record at most the sites the rule finds in the file's base source. So a newly enabled rule, or a widened glob, can record sites that already existed. A moved or new file, and any new site, still have to be fixed. Rules already on at the base, such asrules-of-hooksorno-debugger, are ratcheted as before.The script takes
--base <git-ref>, so CI and a local run use the same command:node lint/check-suppressions.mjs --base origin/main. A bare--baseor a ref the checkout cannot resolve exits 2. A commit that predates the suppressions file runs the path check only, and the script says so.Checks run locally
npm run lintpasses.node lint/check-suppressions.mjs --base origin/mainpasses, and so does the run without a base.npm run typecheckpasses.npx vitest run src/components/useLatestLoad.test.ts src/components/usePagedList.test.tsx --maxWorkers=2: 56 passed atf74a46c4.useLatestLoad.tsabortedabortedloadadded to the effect keysload), killed at 120 sProof
The lint proof was run at
12eb9ae7; the growth table below was re-run atf74a46c4. Every scratch edit was reverted (git statusclean afterwards). Absolute paths in the output are shortened to repo-relative.The lint catches #918's pattern
#918's squash diff. ESLint, with this PR's rule and no suppressions, linted
Dashboard.tsxatf4fa0570^and atf4fa0570. Before #918 the rule fires at line 71. After it, the rule fires at lines 138, 189 and 211, which are #918's three effects. So the per-file count goes from 1 to 3, and the ratchet would have failed #918.On today's code. A second copy of #918's yesterday-close effect, taken verbatim from
f4fa0570, was added toDashboard.tsx. That file has 7 suppressed sites.A1. Effect added:
$ npm run lint
A2. Reverted:
$ npm run lint
A3. The same read through
useLatestLoadinstead:$ npm run lint
Once the count goes over the suppressed 7, ESLint reports every site of the rule in the file, the suppressed ones included.
The growth check on a later PR
node lint/check-suppressions.mjs --base <ref>, with only the suppressions JSON edited.origin/mainis this PR's base.HEADstands in for the next PR's base.origin/mainorigin/mainsrc/components/StockBar.tsx react-hooks/rules-of-hooks: count 0 -> 1origin/mainsrc/components/StockBar.tsx no-debugger: count 0 -> 1origin/mainsrc/components/StockBar.tsx no-restricted-syntax: count 0 -> 1 (the base source has 0)origin/mainsrc/routes/Dashboard.tsx no-restricted-syntax: count 0 -> 8 (the base source has 7)HEADHEADsrc/routes/Dashboard.tsx no-restricted-syntax: count 7 -> 8Rows 4 and 5 were green at
12eb9ae7. They are red now, because the base source caps a new rule's entries. That closes the ceiling the earlier version of this body described: the PR that enables a rule can no longer baseline a fresh violation of it. An entry with no matching violation still fails ESLint itself (npm run lintexits 2).Review round 1 fixes
f74a46c4retry()left the old run live until the effect ranretry()49456cdc, test hardened inf2838704loadthat throws synchronously escaped the hookloadis invoked insidenew Promise(resolve => resolve(...)), so a throw takes the guarded rejection path49456cdc--baseskipped the growth check--baseor an unknown ref exits 2; "predates the file" is told apart from a git errorf74a46c4Findings 2 and 3. Two new tests: "drops the old run's response the moment retry is called, before the new run starts" (the save-then-retry interleaving) and "reports a load that throws before returning a promise as an error, not a crash". Against the hook as at
12eb9ae7, both fail. With the fix, 8 of 8 pass. Mutations: removing the abort inretry()fails the first test, and invokingloadunguarded fails the second. My first version of the retry test passed without the abort, because thenew Promisewrapper adds a microtask.f2838704lets the old run settle fully (asetTimeout(0)) before act ends, and the abort mutation then fails it.Finding 1. The reviewer's repro, plus two controls. Base =
HEAD, where the rule is on for routes and components. "Before fix" runs the check as atf2838704.HEAD, where the rule is on for routes/components)npm run lintf2838704)src/lib/behind a re-export, glob widened, a second async effect added, entry moved and raised 1 -> 2,bannerCache.ts's existing site recordedsrc/lib/review1190NamedEntityPicker.tsx no-restricted-syntax: count 0 -> 2 (the base source has 0)src/farm/**, its existing sites recorded with--suppress-rule(1 new entries)src/farm/useLogoObjectUrl.ts, its entry raised by 1src/farm/useLogoObjectUrl.ts no-restricted-syntax: count 0 -> 2 (the base source has 1)Finding 4.
--baseargument, with Dashboardno-floating-promisesraised 1 -> 2--base HEADsrc/routes/Dashboard.tsx @typescript-eslint/no-floating-promises: count 1 -> 2--base no-such-ref-1190--base no-such-ref-1190 is not a commit this checkout has. Fetch it first.--baseUsage, from web/: node lint/check-suppressions.mjs [--base <git-ref>]